Skip to content

Remove duplicate audience length check#5116

Closed
braginini wants to merge 5 commits intomainfrom
fix/browser-ssh-audience-valiudation
Closed

Remove duplicate audience length check#5116
braginini wants to merge 5 commits intomainfrom
fix/browser-ssh-audience-valiudation

Conversation

@braginini
Copy link
Copy Markdown
Collaborator

@braginini braginini commented Jan 16, 2026

Describe your changes

Issue ticket number and link

Stack

Checklist

  • Is it a bug fix
  • Is a typo/documentation fix
  • Is a feature enhancement
  • It is a refactor
  • Created tests that fail without the change (if possible)

By submitting this pull request, you confirm that you have read and agree to the terms of the Contributor License Agreement.

Documentation

Select exactly one:

  • I added/updated documentation for this change
  • Documentation is not needed for this change (explain why)

Docs PR URL (required if "docs added" is checked)

Paste the PR link from https://github.com/netbirdio/docs here:

https://github.com/netbirdio/docs/pull/__

Summary by CodeRabbit

  • New Features

    • JWT authentication now supports multiple audiences for enhanced flexibility in token validation configuration
  • Tests

    • Added comprehensive test coverage for multi-audience JWT validation scenarios, including acceptance and rejection cases

✏️ Tip: You can customize this high-level summary in your review settings.

@braginini braginini requested a review from lixmal January 16, 2026 12:17
@coderabbitai
Copy link
Copy Markdown
Contributor

coderabbitai Bot commented Jan 16, 2026

Caution

Review failed

The pull request is closed.

📝 Walkthrough

Walkthrough

The changes introduce support for multiple JWT audiences in the authentication system. The JWTConfig.Audience field (string) is replaced with Audiences ([]string) across SSH server configuration, proto definitions, and management server conversion logic, enabling token validation against multiple allowed audiences.

Changes

Cohort / File(s) Summary
Proto Definitions
shared/management/proto/management.proto
Added new repeated string audiences field (tag 5) to JWTConfig message for multi-audience support; existing audience field remains but implicit deprecation via comments.
Management Server Conversion
management/internals/shared/grpc/conversion.go, management/internals/shared/grpc/conversion_test.go
Updated buildJWTConfig() to construct Audiences slice from AuthAudience and optionally appending CLIAuthAudience; added test cases covering single and multiple audience scenarios.
SSH Server Configuration
client/internal/engine_ssh.go
Modified to derive audiences from GetAudiences() with fallback to single Audience, passing as slice to SSH server JWT config initialization.
SSH Server JWT Logic
client/ssh/server/server.go
Replaced Audience field with Audiences slice in JWTConfig; updated validation to require non-empty audiences, adjusted validator construction and claims extractor to use the audiences list.
SSH Proxy & Server Tests
client/ssh/proxy/proxy_test.go, client/ssh/server/jwt_test.go
Updated test initializations from Audience to Audiences slice; added new TestJWTMultipleAudiences() test covering multi-audience validation with dashboard and CLI audiences.

Sequence Diagram(s)

sequenceDiagram
    participant ProtoMgmt as Proto Definition<br/>(management.proto)
    participant Conversion as Management<br/>Conversion Logic
    participant SSHEngine as SSH Engine<br/>(engine_ssh.go)
    participant SSHServer as SSH Server<br/>(server.go)
    participant Validator as JWT Validator

    ProtoMgmt->>ProtoMgmt: Define Audiences field ([]string)
    Conversion->>Conversion: buildJWTConfig() constructs<br/>Audiences slice from<br/>AuthAudience + CLIAuthAudience
    SSHEngine->>SSHEngine: GetAudiences() retrieves<br/>audiences from proto config
    SSHEngine->>SSHServer: Initialize JWT config<br/>with Audiences slice
    SSHServer->>SSHServer: Validate Audiences<br/>not empty
    SSHServer->>Validator: Configure validator with<br/>multiple allowed audiences
    Validator->>Validator: Accept tokens matching<br/>any audience in slice
Loading

Estimated code review effort

🎯 3 (Moderate) | ⏱️ ~20 minutes

Possibly related PRs

Suggested reviewers

  • lixmal
  • mlsmaycon

Poem

🐰 Audiences multiply like carrots in spring,
No longer just one, but a list-ening thing!
From proto to server, the slice finds its way,
Multiple tokens now dance and play!
Hop hop, validate them all! 🎪

✨ Finishing touches
  • 📝 Generate docstrings


📜 Recent review details

Configuration used: defaults

Review profile: CHILL

Plan: Pro

📥 Commits

Reviewing files that changed from the base of the PR and between 1ff7abe and d9b76b4.

⛔ Files ignored due to path filters (1)
  • shared/management/proto/management.pb.go is excluded by !**/*.pb.go
📒 Files selected for processing (7)
  • client/internal/engine_ssh.go
  • client/ssh/proxy/proxy_test.go
  • client/ssh/server/jwt_test.go
  • client/ssh/server/server.go
  • management/internals/shared/grpc/conversion.go
  • management/internals/shared/grpc/conversion_test.go
  • shared/management/proto/management.proto

✏️ Tip: You can disable this entire section by setting review_details to false in your review settings.


Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands and usage tips.

@sonarqubecloud
Copy link
Copy Markdown

@braginini braginini closed this Jan 16, 2026
@braginini braginini deleted the fix/browser-ssh-audience-valiudation branch January 16, 2026 12:18
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

2 participants